feat(integration): ask whether to review pull requests when connecting GitHub - #87
Conversation
There was a problem hiding this comment.
LGTM: Clean, well-tested CLI surface for the connect-time PR-reviews question; decision table, prompt gating, and URL building all check out.
- [nit] F1 src/commands/integration/connect.ts:116 — --pr-reviews/--no-pr-reviews silently ignored for non-GitHub types
Verdict: approve — small, well-factored change; no correctness issues found. Reviewed head b5583ee.
What I checked:
- Decision table (
prReviewsChoiceFromFlags, connect.ts:110-119): opt-out beats opt-in when both flags are passed; interactive-without-flag →'ask'; non-interactive-without-flag →undefined(nothing sent, server default). Matches the PR body's stated semantics and the tests pin all five cases. - Flag parsing:
--no-pr-reviewsis declared as its own boolean option, and the parser (src/args.tskebabToCamel) maps it tonoPrReviews: true— same pattern as the existing--no-browser/noBrowser, so no hidden--no-negation semantics to trip over. - Prompt gating (connect.ts:1080-1089): the question fires only when
canWaitForBrowser(config)is true, which excludes--dry-run,--output json, and non-TTY runs — so scripts and agents never block on the confirm.BACKfrom the prompt propagates out ofconnectTypeand restarts the type picker, consistent with the other steps. The already-connected short-circuit at 1069-1077 runs first, so an existing connection is never re-asked (only--reconnectre-enters, which is reasonable). - URL building (
githubConnectUrl): appends&pr_reviews=on|offtocliConnectUrl, which always already carries?flow=...&workspace=..., so the bare&concat is safe and the values need no encoding. Omitted entirely whenundefined, as the tests verify. - Verified locally: full
npm test(449 pass, 0 fail),npm run typecheck,npm run lintall green on the PR head. (An initial test run right afternpm cishowed transient failures that did not reproduce; two subsequent runs were fully green, and main is green too.)
Finding:
- F1 (nit, connect.ts:116) —
--pr-reviews/--no-pr-reviewspassed with a non-GitHub--type(e.g.--type slack --no-pr-reviews) are silently ignored rather than rejected. The help text scopes them to GitHub and other type-scoped flags behave the same way, so this is consistent with the existing convention — just noting a warning would be friendlier. Not blocking.
The SKILL.md paragraph and help/examples accurately describe the behavior. The one unchecked validation item (UAT round-trip against the nominal server PR) is correctly flagged as human-gated and doesn't affect this CLI-side review.
b5583ee to
380e035
Compare
There was a problem hiding this comment.
LGTM: Re-review at rebased head 380e035: F1 (silent ignore of PR-review flags on non-GitHub types) is fixed with a stderr warning, JSON-safe guard, and test; full diff re-read, all checks green.
Verdict: approve — F1 addressed at head 380e035; no findings remain.
Re-review notes:
• F1 (fixed, verified) — connect.ts:1087-1089 now warns on stderr when --pr-reviews / --no-pr-reviews is passed with a non-GitHub type: --pr-reviews / --no-pr-reviews only apply to --type github; ignored. The prReviewsFlagsGiven guard is correct (true for either flag, false otherwise — pinned by the new test, including a noBrowser non-match case), and the warning is suppressed under --output json and written to stderr, so scripted/JSON consumers are unaffected. Placement inside connectType means it also fires when the type was picked interactively, which matches the comment's rationale.
• The branch was rebased into a single commit, so the prior head b5583ee no longer exists; the full diff vs main is 128 lines and I re-read it in whole. Nothing else changed in substance — decision table, prompt gating, URL building, and help text are as previously reviewed, plus the new SKILL.md paragraph accurately describes the flag behavior.
• Verified locally: npm test (450 pass, 0 fail), npm run typecheck, npm run lint all green at the PR head. As in round 1, the very first test run right after npm ci showed transient failures (18 this time) that vanished on re-run — pre-existing flake in the harness warm-up, unrelated to this PR, but worth a look someday.
Clean fix round — nothing further.
…g GitHub `integration connect --type github` asks one question before opening the browser: review pull requests for production impact on the repositories this connection brings in (default yes). The answer rides the /cli/connect URL as pr_reviews so the console records it on the integration before the repository sync, and the browser never asks a second time. `--no-pr-reviews` opts out and `--pr-reviews` answers yes without the prompt. A non-interactive run without either flag sends nothing, so the server keeps its default and records no answer. The installer needs no change: it already runs this command interactively, so the question appears in the install flow as-is.
8068e20 to
04b8c18
Compare
There was a problem hiding this comment.
LGTM: Prompt reshape to the installer [Y/n] style is correct and well-tested; cancel/EOF map to BACK, no findings.
Verdict: approve — the prompt reshape at head 04b8c18 is correct; no findings. This round's verdict covers the whole PR, not just the delta.
The branch was rebased again (380e035 is gone), so I re-read the full 182-line diff vs main. The only substantive change since the last approval is the prompt mechanics:
• promptYesNoOrBack / parseYesNo (prompt.ts:127-152) — clean readline implementation of the installer's [Y/n] shape. ensureInteractive still guards the non-TTY case; Enter keeps the default; parseYesNo faithfully mirrors ask_yn (first-char match, unrelated answers keep the default) and the new test pins all of that.
• Cancel semantics verified — I checked directly against Node 22 (the repo's runtime): with no SIGINT listener on the interface, Ctrl+C closes readline, so the close handler resolves null → BACK, same as Ctrl+D/EOF. That matches the clack-confirm behavior it replaces — backing out returns to the type picker rather than hanging or hard-exiting. The double-resolve (question callback, then close in finally) is harmless since the promise is already settled.
• askPrReviews (connect.ts:135-147) — the note box is replaced with a single plain stdout line before the question, which does read like install.sh's neighbouring Connect GitHub? [Y/n] lines. Only reachable in the interactive 'ask' path, so JSON/non-TTY runs are unaffected.
• Everything previously reviewed is unchanged in substance: the flag decision table, opt-out-wins precedence, non-GitHub warning (F1 fix from round 2, still present at connect.ts:1089-1091), URL building, help text, and the SKILL.md paragraph.
Verified locally: npm test 453 pass / 0 fail across two runs, npm run typecheck and npm run lint green. Third round in a row where the very first post-npm ci test run showed transient failures (18) that vanish on re-run — still pre-existing harness warm-up flake, unrelated to this PR, but it's now a pattern worth filing.
polylane integration connect --type githubnow asks one question before it opens the browser: review pull requests for production impact on the repositories this connection brings in (default yes). Teams that do not want those reviews say so once, in the terminal, instead of turning them off repository by repository afterwards.What & why
Every repository a GitHub connection brings in is reviewed for production impact as soon as it syncs. The server side (coreplanelabs/nominal#2087, on UAT) records a connect-time answer on the integration and stamps it onto each repository the sync creates. This PR is the CLI surface for that answer. Validation ledger across both PRs: coreplanelabs/nominal#2089.
The question is asked exactly where the CLI already hands off to the browser, so the installer needs no change:
install.shruns this command interactively and the question appears in the install flow as-is, between the script's ownConnect GitHub? [Y/n]andConnect Slack? [Y/n]. It is asked in the same[Y/n]shape as those lines (Enter = yes, only annanswer opts out), not as a clack confirm, so the installer reads as one flow. Two flags answer without the prompt for agents and scripts. The answer rides the existing/cli/connectURL aspr_reviews, and the console records it before the repository sync starts; the browser never asks a second time.Validated end to end on UAT before release through the onboarding lab with this branch's bundle (nominal#2120 adds the knob): runs
260904001733e7vy(answer no →pr_reviews=off, integration opted out) and260904001941emf0(default yes →pr_reviews=on).Tour
1. Deciding whether to ask
--no-pr-reviewsand--pr-reviewsanswer without the prompt; an interactive run asks; a non-interactive run without a flag sends nothing, so the server keeps its default and records no answer.Look for: opt-out wins if both flags are passed;
undefined(not asked) and'on'(asked, kept) are deliberately different values.cli/src/commands/integration/connect.ts
Lines 107 to 120 in 04b8c18
2. The flag guard and the URL
prReviewsFlagsGivenlets a non-GitHub type warn that the flags are ignored (a warning, not an error, because the type can be picked interactively after the flag was typed).githubConnectUrlappendspr_reviewsonly when there is an answer;cliConnectUrlalways carries?flow=…&workspace=…, so the bare&is safe.cli/src/commands/integration/connect.ts
Lines 122 to 133 in 04b8c18
3. The installer's
[Y/n]shapeparseYesNoisinstall.sh'sask_ynin TypeScript: Enter keeps the default, only an answer starting withn(ory, for a default-no question) flips it, anything else keeps the default.promptYesNoOrBackprints<question> [Y/n]and reads one line from stdin, whichrun_ttyin the installer hands the terminal to; a closed stream isBACK.cli/src/utils/prompt.ts
Lines 128 to 151 in 04b8c18
4. The question
One plain sentence saying what a review is and where to change it later, then the
[Y/n]line. Cancel isBACK, which restarts the type picker like every other step.cli/src/commands/integration/connect.ts
Lines 135 to 149 in 04b8c18
5. Where it runs in the GitHub connect
After the already-connected short-circuit (an existing connection is never re-asked) and before the browser opens.
canWaitForBrowseris the interactivity test, so--output json,--dry-run, and non-interactive runs never see the prompt. A non-GitHub type with either flag prints the one-line warning and carries on.cli/src/commands/integration/connect.ts
Lines 1087 to 1101 in 04b8c18
6. Flags and help
Both flags are scoped to GitHub in their descriptions, and the example list gains the opt-out form.
cli/src/commands/integration/connect.ts
Lines 1157 to 1174 in 04b8c18
7. Tests
Pure-function tests for the decision table, the flag guard, the URL shape, and the
[Y/n]parser.https://github.com/coreplanelabs/cli/blob/04b8c18e6329867751cf4cfe0afdbdfb43689981/test/prompt-yes-no.test.ts#L5-L24
8. Remaining changes
test/integration-connect-pr-reviews.test.ts— decision table, flag guard, URL.skill/SKILL.md— one paragraph telling agents about the question and the two flags (regenerated intosrc/generated/skill.tsby codegen).Decisions
install.sh. The installer is a thin shim over this CLI by design (AGENTS.md), and it already runs this command interactively; asking here means the question ships with a CLI release, needs no website deploy, and adds no shell-side version probe.[Y/n]shape, not a clack confirm. The first UAT run showed a boxed note and a clack Yes/No wedged between two[Y/n]lines; inside the installer the question must read like its neighbours, so it now uses the same prompt grammar and the same answer rule asask_yn.--pr-reviewsexists alongside--no-pr-reviews. A non-interactive run that wants to record "yes, asked" needs a way to say so; without the positive flag the server could not tell "kept on" from "never asked".Validation
--no-pr-reviewsyieldsoffregardless of interactivity —integration-connect-pr-reviews.test.ts--pr-reviewsyieldsonregardless of interactivityask; no flag, non-interactive yieldsundefinedprReviewsFlagsGivenis true for either flag and false otherwise (the non-GitHub warning's guard)pr_reviews=off/pr_reviews=onwhen answered and omits the parameter when not askedparseYesNo: Enter keeps the default,n…no,y…yes, anything else keeps the default (mirrorsask_yn, verified against the livepolylane.com/install.sh) —prompt-yes-no.test.tsSyntaxError: ... does not provide an export named 'githubConnectUrl') and passes with itnpm test(full suite),npm run typecheck,npm run lintall green260904001733e7vyand260904001941emf0[Y/n]line in the installer transcript (same commands as the ledger).